Skip to content

Refactor sudo handling for package installation#83

Closed
tannevaled wants to merge 1 commit into
pkgxdev:mainfrom
tannevaled:dev
Closed

Refactor sudo handling for package installation#83
tannevaled wants to merge 1 commit into
pkgxdev:mainfrom
tannevaled:dev

Conversation

@tannevaled
Copy link
Copy Markdown

proposal to fix sudo handling

Copilot AI review requested due to automatic review settings May 4, 2026 12:15
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR proposes to refactor how pkgm decides when to invoke pkgx directly vs wrapping it with sudo, specifically around installs targeting /usr/local.

Changes:

  • Introduces explicit isRoot and SUDO_USER detection in query_pkgx.
  • Emits a warning when installing to /usr/local as root without being invoked via sudo.
  • Changes the conditions under which /usr/bin/sudo is used and how the sudo arguments are constructed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkgm.ts Outdated
Comment on lines +311 to +319
const needs_sudo_backwards =
install_prefix().string == "/usr/local" && !isRoot && !sudoUser;

let cmd = pkgx;

if (needs_sudo_backwards) {
cmd = "/usr/bin/sudo";
const user = Deno.env.get("USER")!;
args.unshift("-u", user, pkgx);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one seems like the problem; we need drop privileges so we don't mess up a user's permissions, i believe.

Comment thread pkgm.ts Outdated
Comment on lines 315 to 323

if (needs_sudo_backwards) {
cmd = "/usr/bin/sudo";
const user = Deno.env.get("USER")!;
args.unshift("-u", user, pkgx);
}

const proc = new Deno.Command(cmd, {
args: [...args, "--json=v1"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a lateral fix that was either always needed, or isn't now needed.

Comment thread pkgm.ts Outdated

if (needs_sudo_backwards) {
cmd = "/usr/bin/sudo";
const user = Deno.env.get("USER")!;
Comment thread pkgm.ts Outdated
Copilot AI review requested due to automatic review settings May 4, 2026 16:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkgm.ts Outdated
Comment on lines +312 to +322
const needs_sudo_backwards =
prefix == "/usr/local" && !isRoot && !sudoUser;

let cmd = pkgx;

if (needs_sudo_backwards) {
cmd = "/usr/bin/sudo";
const user = Deno.env.get("USER")!;
args.unshift("-u", user, pkgx);
}

Comment thread pkgm.ts Outdated
Comment on lines +319 to +320
const user = Deno.env.get("USER")!;
args.unshift("-u", user, pkgx);
Comment thread pkgm.ts Outdated
Comment on lines 316 to 324

if (needs_sudo_backwards) {
cmd = "/usr/bin/sudo";
const user = Deno.env.get("USER")!;
args.unshift("-u", user, pkgx);
}

const proc = new Deno.Command(cmd, {
args: [...args, "--json=v1"],
Comment thread pkgm.ts Outdated

if (needs_sudo_backwards) {
cmd = "/usr/bin/sudo";
const user = Deno.env.get("USER")!;
Comment thread pkgm.ts Outdated
Copilot AI review requested due to automatic review settings May 5, 2026 06:48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkgm.ts Outdated
Comment on lines +312 to +320
const needs_sudo_backwards =
prefix == "/usr/local" && !isRoot && !sudoUser;

let cmd = pkgx;

if (needs_sudo_backwards) {
if (!Deno.env.get("SUDO_USER")) {
if (Deno.uid() == 0) {
console.error(
"%cwarning",
"color:yellow",
"installing as root; installing via `sudo` is preferred",
);
}
cmd = pkgx;
} else {
args.unshift("-u", Deno.env.get("SUDO_USER")!, pkgx);
}
cmd = "/usr/bin/sudo";
const user = Deno.env.get("USER")!;
args.unshift("-u", user, pkgx);
Comment thread pkgm.ts Outdated
Comment on lines 318 to 321
cmd = "/usr/bin/sudo";
const user = Deno.env.get("USER")!;
args.unshift("-u", user, pkgx);
}
Comment thread pkgm.ts Outdated
@tannevaled
Copy link
Copy Markdown
Author

tannevaled commented May 11, 2026

diff --git a/pkgm.ts b/pkgm.ts
index f3f08c3..53dc13f 100755
--- a/pkgm.ts
+++ b/pkgm.ts
@@ -297,25 +297,43 @@ async function query_pkgx(
   set("PKGX_DIST_URL");
   set("XDG_DATA_HOME");
 
-  const needs_sudo_backwards = install_prefix().string == "/usr/local";
-  let cmd = needs_sudo_backwards ? "/usr/bin/sudo" : pkgx;
-  if (needs_sudo_backwards) {
-    if (!Deno.env.get("SUDO_USER")) {
-      if (Deno.uid() == 0) {
+  const isRoot = Deno.uid() == 0;
+  const sudoUser = Deno.env.get("SUDO_USER");
+  const prefix = install_prefix().string;
+  const isSystemPrefix = prefix == "/usr/local";
+
+  let cmd = pkgx;
+  let cmd_args = args;
+
+  if (isSystemPrefix) {
+    if (isRoot && sudoUser) {
+      // Drop privileges so pkgx writes its cache as the invoking user, not root.
+      // But only if pkgx is reachable from sudoUser — otherwise the inner sudo
+      // aborts with "unable to execute …: Permission denied" (pkgxdev/pkgm#68).
+      const reachable = pkgx_reachable_as(pkgx, sudoUser);
+      if (reachable) {
+        cmd = "/usr/bin/sudo";
+        cmd_args = ["-u", sudoUser, reachable, ...args];
+        // Override HOME, or pkgx will cache back under /root/ where sudoUser
+        // can't reach it on the next invocation.
+        const home = user_home(sudoUser);
+        if (home) env.HOME = home;
+      } else if (Deno.env.get("PKGM_DEBUG")) {
         console.error(
-          "%cwarning",
-          "color:yellow",
-          "installing as root; installing via `sudo` is preferred",
+          `pkgm: \`pkgx\` at ${pkgx} is not reachable as ${sudoUser}; running it as root`,
         );
       }
-      cmd = pkgx;
-    } else {
-      args.unshift("-u", Deno.env.get("SUDO_USER")!, pkgx);
+    } else if (isRoot) {
+      console.error(
+        "%cwarning",
+        "color:yellow",
+        "installing as root; installing via `sudo` is preferred",
+      );
     }
   }
 
   const proc = new Deno.Command(cmd, {
-    args: [...args, "--json=v1"],
+    args: [...cmd_args, "--json=v1"],
     stdout: "piped",
     env,
     clearEnv: true,
@@ -766,6 +784,58 @@ function install_prefix() {
   }
 }
 
+function user_home(user: string): string | undefined {
+  // getent is the portable lookup on Linux; on macOS getent is absent but the
+  // /root/.pkgx scenario this guards against doesn't arise there in practice.
+  try {
+    const out = new Deno.Command("/usr/bin/getent", {
+      args: ["passwd", user],
+    }).outputSync();
+    if (!out.success) return undefined;
+    const fields = new TextDecoder().decode(out.stdout).trim().split(":");
+    return fields[5] || undefined;
+  } catch {
+    return undefined;
+  }
+}
+
+function pkgx_reachable_as(current: string, user: string): string | undefined {
+  if (reachable_as(current, user)) return current;
+
+  const home = user_home(user);
+  if (home) {
+    // Versioned pkgx.sh layout: ~/.pkgx/pkgx.sh/v<x.y.z>/bin/pkgx — pick the highest.
+    const root = join(home, ".pkgx/pkgx.sh");
+    if (existsSync(root)) {
+      let best: { v: SemVer; path: string } | undefined;
+      for (const entry of Deno.readDirSync(root)) {
+        if (!entry.isDirectory || !entry.name.startsWith("v")) continue;
+        try {
+          const v = new SemVer(entry.name.slice(1));
+          const path = join(root, entry.name, "bin/pkgx");
+          if (!existsSync(path)) continue;
+          if (!best || v.gt(best.v)) best = { v, path };
+        } catch { /* skip malformed version dir */ }
+      }
+      if (best) return best.path;
+    }
+    const local = join(home, ".local/bin/pkgx");
+    if (existsSync(local)) return local;
+  }
+  if (existsSync("/usr/local/bin/pkgx")) return "/usr/local/bin/pkgx";
+  return undefined;
+}
+
+function reachable_as(p: string, user: string): boolean {
+  // Conservative heuristic: private home dirs are typically mode 700, so a
+  // path under another user's home is unreachable. System paths and the
+  // user's own home are assumed reachable.
+  if (p.startsWith("/root/")) return user === "root";
+  const m = p.match(/^\/home\/([^/]+)\//);
+  if (m) return m[1] === user;
+  return true;
+}
+
 function dev_stub_text(selfpath: string, bin_prefix: string, name: string) {
   if (selfpath.startsWith("/usr/local") && selfpath != "/usr/local/bin/dev") {
     return `

Restore the privilege drop when running as root via sudo (so pkgx caches
stay user-owned), but only when the resolved pkgx binary is actually
reachable from $SUDO_USER. Falls back to running pkgx as root rather
than crashing the inner sudo with "Permission denied" when pkgx lives
under /root/.pkgx/ (pkgxdev#68).

- Restore drop-privilege behaviour for `sudo pkgm` (fixes the
  regression flagged by jhheider on pkgxdev#83).
- Resolve an alternative pkgx under $SUDO_USER's home / /usr/local
  when the current path is unreachable to $SUDO_USER.
- Override HOME so pkgx caches under the invoking user's tree.
- Stop mutating `args` so the args[0] lookup at line 341 keeps working.
- Avoid the `Deno.env.get("USER")!` non-null assertion crash.
- Call install_prefix() once (it has filesystem side effects).
- Keep the visible log surface unchanged: only the pre-existing
  "installing as root" warning fires by default; the new
  "pkgx unreachable" diagnostic is gated behind PKGM_DEBUG.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants